Repository navigation
perf(desktop,host-service): enqueue workspace creation off pooled sockets and batch the host-service client - #6304
Conversation
…mLink #3879 unbatched this client because httpBatchLink buffered the whole batch behind its slowest procedure. httpBatchStreamLink streams each result as it completes, so same-tick calls share one socket and one CORS preflight without the slowest-in-batch latency. Every renderer client shares Chromium's 6-per-origin pool to host-service, so pooled sockets are the scarce resource.
… the eventBus The synchronous create mutation holds a pooled HTTP socket for the whole create — minutes of git worktree work on big repos — and relay-fronted hosts cap request exchanges at 30s, so remote creates of large repos failed outright. workspaces.createEnqueued validates, returns immediately, runs the real create in the background, and broadcasts a workspace:create-settled event carrying what the synchronous response used to (canonical id + launched terminals/agents for the pane-layout seed). The renderer subscribes before enqueueing, keeps the whole optimistic flow (sidebar row, empty pane seed, failure record), and falls back to the legacy synchronous create on hosts without the procedure. A lost settled event (host restart mid-create) degrades to a row-existence check instead of tearing down a workspace that's actually there. CLI/MCP/SDK/automations keep the synchronous create. Session creates stay synchronous too (no git work) but are now bounded, as is the enqueue call itself — per-case budgets, not a blanket number. Also asserts the persisted session row in the create-on-attach test (review follow-up from #6299).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe host service now supports asynchronous workspace creation through ChangesWorkspace creation settlement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DesktopWorkspaceCreation
participant CreateEnqueued
participant CreateCaller
participant EventBus
DesktopWorkspaceCreation->>CreateEnqueued: enqueue workspace creation
CreateEnqueued-->>DesktopWorkspaceCreation: return workspace ID
CreateEnqueued->>CreateCaller: invoke workspaces.create asynchronously
CreateCaller->>EventBus: broadcast workspace:create-settled
EventBus-->>DesktopWorkspaceCreation: deliver settlement payload
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/lib/host-service-client.ts (1)
23-33: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDecouple long-running host tRPC mutations from same-tick batches.
httpBatchStreamLinkallowsworkspaces.createSessionand the long-runningworkspaces.createfallback to join a single/trpcrequest. That request goes through relay2’s HTTP exchange backed byHostTunnel.proxyHttp, plus relay2’sEXCHANGE_TIMEOUT_MS = 30_000. The relay can return BAD_GATEWAY when the shared exchange times out even after faster batch entries stream back. Route these long-running mutations through an unbatched client, or add amaxItems: 1/single-call path that is not affected by other same-tick calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/lib/host-service-client.ts` around lines 23 - 33, Update the host-service client setup around httpBatchStreamLink so long-running workspaces.createSession and fallback workspaces.create mutations cannot share same-tick HTTP batches with other calls. Route those mutations through a separate unbatched client or an equivalent single-call path with maxItems: 1, while preserving streaming batching for unrelated host-service requests.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts (2)
114-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClear the settle timer after the race resolves.
The
setTimeoutis never cleared. When the settled event wins the race, the timer stays pending for 10 minutes per create. It resolves an already-settled promise, so behavior is correct, but the timer holds its closure until it fires.♻️ Proposed change
- const outcome = await Promise.race([ - settled, - new Promise<"timeout">((resolve) => - setTimeout(() => resolve("timeout"), CREATE_SETTLE_TIMEOUT_MS), - ), - ]); + let settleTimer: ReturnType<typeof setTimeout> | undefined; + const outcome = await Promise.race([ + settled, + new Promise<"timeout">((resolve) => { + settleTimer = setTimeout(() => resolve("timeout"), CREATE_SETTLE_TIMEOUT_MS); + }), + ]).finally(() => { + if (settleTimer !== undefined) clearTimeout(settleTimer); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts` around lines 114 - 119, Update the Promise.race flow in useWorkspaceCreates to retain the timer handle and clear it immediately after the race resolves, regardless of whether settled or the timeout wins. Preserve the existing timeout result and settled-event behavior.
69-75: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the legacy create fallback.
workspaces.createEnqueuedcallsvalidateAgentLaunchEffort, whose unknown agentNOT_FOUNDhas a different message. RestrictisMissingProcedureErrorto botherror.data?.code === "NOT_FOUND"and/no procedure found on path/ibefore falling back toclient.workspaces.create.mutate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/host-service/test/integration/workspace-create-enqueued.integration.test.ts`:
- Around line 67-70: Add an immediate post-mutation assertion in the workspace
creation enqueue test to verify that settled remains empty before awaiting
waitFor. Preserve the existing waitFor assertion to confirm the broadcast
eventually occurs.
---
Outside diff comments:
In `@apps/desktop/src/renderer/lib/host-service-client.ts`:
- Around line 23-33: Update the host-service client setup around
httpBatchStreamLink so long-running workspaces.createSession and fallback
workspaces.create mutations cannot share same-tick HTTP batches with other
calls. Route those mutations through a separate unbatched client or an
equivalent single-call path with maxItems: 1, while preserving streaming
batching for unrelated host-service requests.
---
Nitpick comments:
In `@apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts`:
- Around line 114-119: Update the Promise.race flow in useWorkspaceCreates to
retain the timer handle and clear it immediately after the race resolves,
regardless of whether settled or the timeout wins. Preserve the existing timeout
result and settled-event behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ffc89bb9-ab19-4920-bdae-91d600bfae9c
📒 Files selected for processing (12)
apps/desktop/src/renderer/lib/host-service-client.tsapps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.tspackages/host-service/src/app.tspackages/host-service/src/events/event-bus.tspackages/host-service/src/events/types.tspackages/host-service/src/terminal/terminal.create-on-attach.node-test.tspackages/host-service/src/trpc/index.tspackages/host-service/src/trpc/router/workspaces/workspaces.tspackages/host-service/test/helpers/createTestHost.tspackages/host-service/test/integration/workspace-create-enqueued.integration.test.tspackages/workspace-client/src/index.tspackages/workspace-client/src/lib/eventBus.ts
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/test/integration/workspace-create-enqueued.integration.test.ts">
<violation number="1" location="packages/host-service/test/integration/workspace-create-enqueued.integration.test.ts:70">
P2: This test claims to verify "returns immediately" but never actually asserts the enqueue settled asynchronously. If createEnqueued regresses to run the create inline (awaiting it), `settled` would already be non-empty when waitFor runs, so the test still passes and the core async contract of this PR goes unguarded. Assert `settled.length === 0` immediately after the mutation resolves (safe — the background create can't complete in the same tick) to lock in the non-blocking behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // round-trip is milliseconds while the worktree add takes hundreds, | ||
| // so nothing may have settled by the time the response resolves. | ||
| expect(settled.length).toBe(0); | ||
| await waitFor(() => settled.length > 0); |
There was a problem hiding this comment.
P2: This test claims to verify "returns immediately" but never actually asserts the enqueue settled asynchronously. If createEnqueued regresses to run the create inline (awaiting it), settled would already be non-empty when waitFor runs, so the test still passes and the core async contract of this PR goes unguarded. Assert settled.length === 0 immediately after the mutation resolves (safe — the background create can't complete in the same tick) to lock in the non-blocking behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/test/integration/workspace-create-enqueued.integration.test.ts, line 70:
<comment>This test claims to verify "returns immediately" but never actually asserts the enqueue settled asynchronously. If createEnqueued regresses to run the create inline (awaiting it), `settled` would already be non-empty when waitFor runs, so the test still passes and the core async contract of this PR goes unguarded. Assert `settled.length === 0` immediately after the mutation resolves (safe — the background create can't complete in the same tick) to lock in the non-blocking behavior.</comment>
<file context>
@@ -0,0 +1,125 @@
+ expect(enqueue.workspaceId).toBe(id);
+ // The enqueue response must not wait for the create: no settled
+ // broadcast may have happened synchronously with the mutation.
+ await waitFor(() => settled.length > 0);
+
+ const event = settled[0];
</file context>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/test/integration/workspace-create-enqueued.integration.test.ts">
<violation number="1" location="packages/host-service/test/integration/workspace-create-enqueued.integration.test.ts:71">
P2: This timing assertion races the background create: `settled.length` must still be 0 at the instant the enqueue mutate resolves, but the real create runs on an async background path and can complete (and broadcast `workspace:create-settled`) before the mutate promise settles on a slow/contended CI machine, producing a spurious failure. Consider asserting the enqueue response itself (e.g. it returns the client-minted id) and relying on the existing `waitFor(() => settled.length > 0)` to verify settlement, rather than asserting an exact timing zero.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // The enqueue response must not wait for the create: the mutation | ||
| // round-trip is milliseconds while the worktree add takes hundreds, | ||
| // so nothing may have settled by the time the response resolves. | ||
| expect(settled.length).toBe(0); |
There was a problem hiding this comment.
P2: This timing assertion races the background create: settled.length must still be 0 at the instant the enqueue mutate resolves, but the real create runs on an async background path and can complete (and broadcast workspace:create-settled) before the mutate promise settles on a slow/contended CI machine, producing a spurious failure. Consider asserting the enqueue response itself (e.g. it returns the client-minted id) and relying on the existing waitFor(() => settled.length > 0) to verify settlement, rather than asserting an exact timing zero.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/test/integration/workspace-create-enqueued.integration.test.ts, line 71:
<comment>This timing assertion races the background create: `settled.length` must still be 0 at the instant the enqueue mutate resolves, but the real create runs on an async background path and can complete (and broadcast `workspace:create-settled`) before the mutate promise settles on a slow/contended CI machine, producing a spurious failure. Consider asserting the enqueue response itself (e.g. it returns the client-minted id) and relying on the existing `waitFor(() => settled.length > 0)` to verify settlement, rather than asserting an exact timing zero.</comment>
<file context>
@@ -65,8 +65,10 @@ describe("workspaces.createEnqueued integration", () => {
+ // The enqueue response must not wait for the create: the mutation
+ // round-trip is milliseconds while the worktree add takes hundreds,
+ // so nothing may have settled by the time the response resolves.
+ expect(settled.length).toBe(0);
await waitFor(() => settled.length > 0);
</file context>
- Bound the backstop row-existence probe (15s) so a wedged transport can't leave the create pending past the backstop. - Clear the 10-minute backstop timer once the settled event wins the race — bulk creates were each leaving an armed timer behind. - A timed-out enqueue may still have reached the host: fall through to the settled event / backstop instead of tearing down a workspace the host is actually creating. Network errors still fail fast. - Same for session creates: on timeout, probe for the row before recording a failure the workspace:changed broadcast would contradict. - Include the correlation id in the host-side create-failed warning.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts">
<violation number="1" location="apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts:306">
P1: A timeout after row insertion but before launch completion writes an empty pane layout while host still creates requested terminal/agent sessions. Keep recovery pending until launch metadata is available, or provide a settled/recovery result containing those launches.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ) { | ||
| return { | ||
| workspace: { id: workspaceId, projectId: null }, | ||
| terminals: [], |
There was a problem hiding this comment.
P1: A timeout after row insertion but before launch completion writes an empty pane layout while host still creates requested terminal/agent sessions. Keep recovery pending until launch metadata is available, or provide a settled/recovery result containing those launches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts, line 306:
<comment>A timeout after row insertion but before launch completion writes an empty pane layout while host still creates requested terminal/agent sessions. Keep recovery pending until launch metadata is available, or provide a settled/recovery result containing those launches.</comment>
<file context>
@@ -258,11 +287,28 @@ export function useWorkspaceCreates(): UseWorkspaceCreatesApi {
+ ) {
+ return {
+ workspace: { id: workspaceId, projectId: null },
+ terminals: [],
+ agents: [],
+ };
</file context>
A probe transport failure must not read as "row absent" (only NOT_FOUND does), and a client-side session timeout can race a create that hasn't inserted its row yet — probe a few times before recording a failure the workspace:changed broadcast would contradict. The empty-pane-seed recovery is deliberate: launches aren't recoverable without the settled event, and session auto-adopt heals the panes on open.
Full surface tour + stress verification (CDP, real UI journeys)Beyond the before/after measurements in the description, every surface this PR touches was driven end-to-end on the built dev app:
Suites under their sanctioned runners: host-service 1102 pass (incl. 4 enqueue integration tests), desktop 2650 pass, workspace-client 13 pass. All cubic/CodeRabbit findings addressed in follow-up commits (bounded backstop probe, cleared backstop timer, enqueue-timeout fall-through instead of teardown, session-timeout spaced row probes, tri-state probe so transport failures don't read as "row absent", correlation id in the host-side failure log). One deliberate trade-off documented in-code: event-loss recovery seeds an empty pane layout — launches aren't recoverable without the event, and session auto-adopt heals the panes on open (verified in the reload journey above). All scratch workspaces created during verification were destroyed (worktrees + branches). |
Follow-up to #6299 (terminal create-on-attach). That PR fixed the worst victim of Chromium's 6-per-origin socket pool to host-service; this one reduces the pressure at its two biggest sources. Both changes are proven with before/after CDP measurements against the real dev app.
Summary
workspaces.createno longer holds a pooled socket for the whole create. Newworkspaces.createEnqueuedvalidates, returns immediately, runs the real create in the background, and broadcasts aworkspace:create-settledeventBus event carrying what the synchronous response used to (canonical workspace id + launched terminals/agents for the pane-layout seed). The desktop renderer switches to it; CLI/MCP/SDK/automations keep the synchronouscreateuntouched.httpBatchStreamLink, which streams each result as it completes, addressing the slowest-in-batch latency that motivated the original unbatching in perf(desktop): unbatch host-service tRPC client #3879.CDP evidence (real UI journeys, dev desktop app)
Create hold time — the marquee number. Same journey both times: New Workspace composer → demo-app project → prompt → submit, with all renderer→host requests timed via CDP Network events:
POST workspaces.createPOST workspaces.createEnqueuedOn demo-app the create is ~4s; on a monorepo it's minutes of
git worktree add— all of it previously pinning 1 of 6 sockets. On relay-fronted hosts the synchronous create exceeded the relay's 30s exchange cap outright (apps/relay/src/tunnel.ts:64), so remote creates of large repos could not succeed at all; the enqueue variant fits comfortably.End-to-end UX verified after the change: workspace appears in the sidebar instantly, navigation lands on it, the AI-generated branch name is applied, and the agent pane is seeded from the settled event and starts executing its prompt (screenshots captured). Scratch workspaces then destroyed cleanly through the same host.
Batching, dashboard route (6 pollers active), 60s windows: 72 → 66 requests/min, preflights 36 → 32, with same-tick batches visibly collapsing (
terminalAgents.list,pullRequests.getByWorkspacespairs). Honest read: the steady-state win is modest because independent poll intervals rarely share a tick — the real value is same-tick fan-outs (multi-hostuseQueries, boot bursts) and that it removes the per-call socket+preflight floor with no latency downside.How It Works
createEnqueuedreuses the entire existingcreateprocedure via a server-side caller (createCallerFactory) — zero changes to the 700-line create body. The settled event goes through a new typedEventBus.broadcastWorkspaceCreateSettled, following the existing lifecycle-event shapes.useWorkspaceCreates.submitsubscribes toworkspace:create-settledfor the client-minted id before enqueueing (no missed-event race), and keeps the existing optimistic flow untouched — sidebar row, empty pane seed,failedWorkspaceCreatesrecord,workspaceTransactionspending UI all behave exactly as before, now driven by the event payload.Testing
bun run typecheck,bun run lintcd packages/host-service && bun test— 1102 pass, 0 fail, including 3 new integration tests forcreateEnqueued(immediate return + settled event with persisted row; missing-id rejection; mid-create failure broadcastsok:falsewith the error)Known Limitations / Follow-ups
WorkspaceCreatingStateprogress UI is still estimate-based; the settled event now provides the terminal signal, and intermediate progress events (worktree add started, agents dispatched) are a natural next step on the same channel.toast.promiseoverPromise.all(handles)) work unchanged — each handle'scompletednow resolves from its event — but weren't individually re-verified beyond the single-create journey.Summary by cubic
Makes workspace creation async and brings back streaming batched tRPC calls to reduce host-service socket pressure and prevent relay timeouts. Adds safer timeout recovery so slow creates and sessions don’t get marked failed.
New Features
workspaces.createEnqueued: validates, returns immediately, runs the real create in the background, and emitsworkspace:create-settled(canonical workspace id, terminals, agents).create.httpBatchStreamLinkfrom@trpc/clientto batch same-tick calls and stream results, reducing preflights and socket usage.broadcastWorkspaceCreateSettled; renderer subscribes before enqueue. Types exported via@superset/workspace-client.Bug Fixes
Written for commit b197d2d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests